-
Notifications
You must be signed in to change notification settings - Fork 308
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add shrink_to_fit
to Array
#1141
base: master
Are you sure you want to change the base?
Conversation
src/impl_owned_array.rs
Outdated
/// assert_eq!(a, b); | ||
/// ``` | ||
pub fn shrink_to_fit(&mut self) { | ||
self.to_default_stride(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to default stride is an expensive operation - so we can probably avoid it on an array that is already perfectly fit.
We do prefer to not lean too strongly on a default memory order if possible. Any contiguous array should not have its memory order thrown around. I would even prefer if an array keeps its memory order the same, if it can.
src/impl_owned_array.rs
Outdated
|
||
/// Convert from any stride to the default stride | ||
pub fn to_default_stride(&mut self) { | ||
let view_mut = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about all the details of this method, but I think it's better to take a step back and see if there isn't already some functionality that can do this - maybe .to_shape()?
shrink_to_fit is a good idea - should it modify array memory layout? When should it modify? Some more description could help us get a good design. |
shrink_to
fit to OwendRepr
Arrayshrink_to_fit
to Array
Thanks for your the code review 😀
For example, I think that changing the memory order is necessary in the following cases. let a = array![[[0, 1, 2], [3, 4, 5], [6, 7, 8]], [[9, 10, 11], [12, 13, 14], [15, 16, 17]], [[18, 19, 20], [21, 22, 23], [24, 25, 26]]];
let a = a.slice_move(s![.., 1.., ..]);
// a ->
// [[[3, 4, 5],
// [6, 7, 8]],
//
// [[12, 13, 14],
// [15, 16, 17]],
//
// [[21, 22, 23],
// [24, 25, 26]]], shape=[3, 2, 3], strides=[9, 3, 1], layout=c (0x4), const ndim=3 I think the memory order would be as follows if we apply before shrink_to_fit
after shrink_to_fit
On the other hand, in the following case, it looks like you can do let a = array![[[0, 1, 2], [3, 4, 5], [6, 7, 8]], [[9, 10, 11], [12, 13, 14], [15, 16, 17]], [[18, 19, 20], [21, 22, 23], [24, 25, 26]]];
let a = a.slice_move(s![..2, .., ..]);
// a ->
// [[[0, 1, 2],
// [3, 4, 5],
// [6, 7, 8]],
//
// [[9, 10, 11],
// [12, 13, 14],
// [15, 16, 17]]], shape=[2, 3, 3], strides=[9, 3, 1], layout=Cc (0x5), const ndim=3 I think the only time you can apply |
I'm back next week |
Apparently not back so fast, I apologize. |
Don't worry about it. We all have busy times 😀 |
What is the current status of this PR? |
Well, I want to come back to this and it would be the first thing I'd work on in ndarray |
@bokutotu If I you are still interested, I would like to try to pick this up again. What would be really helpful would be to remove the formatting-related part of the diff, just so that is easier to review. Other than that, I suspect that all of the feedback provided so far has already been addressed. If you do reboot this, please try to ignore all the Clippy and deprecation issues for now. We will have to get the master branch into shape elsewhere. |
@adamreichold Thanks for your reply. I would like to make a commit to delete the diffs once they are out in cargo fmt. Please wait a bit as work is taking up a lot of my time right now. |
#427
shrink_to_fit
toArray<A, D>
to_default_strid
toArray<A, D>
offset_from_data
toArray<A, D>
shrink_to_fit
toOwnedRepr